Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: mark background job for storage statistics as time insensitive #517

Closed
wants to merge 1 commit into from
Closed

feat: mark background job for storage statistics as time insensitive #517

wants to merge 1 commit into from

Conversation

kesselb
Copy link
Collaborator

@kesselb kesselb commented Nov 1, 2023

No description provided.

@kesselb kesselb added this to the Nextcloud 28 milestone Nov 1, 2023
@kesselb kesselb self-assigned this Nov 1, 2023
@kesselb
Copy link
Collaborator Author

kesselb commented Nov 1, 2023

/backport to stable27

@kesselb
Copy link
Collaborator Author

kesselb commented Nov 1, 2023

/backport to stable26

@kesselb kesselb force-pushed the mark-background-job-as-time-insensitive branch from cbae449 to 81bf9ab Compare November 1, 2023 16:15
@joshtrichards
Copy link
Member

My understanding of TIME_INSENSITIVE is that it's a no-op unless explicitly enabled by the admin (by setting maintenance_window_start)

This change may initially confuse a few people that happen to be using maintenance_window_start and are already polling storage stats, but seems mostly fine in terms of backwards compatibility.

It's a little weird that if someone configures maintenance_window_start that job_interval_storage_stats will seem to essentially be ignored if the interval is >4 hours, but I also don't have a good alternative. Adding a warning to the occ command isn't practical since maintenance_window_start could get enabled after the fact. It may just be that we add a note to the doc (README currently) about the interaction between the two settings...

joshtrichards
joshtrichards approved these changes Nov 1, 2023
View reviewed changes
Copy link
Member

@joshtrichards joshtrichards left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine by me as long as a note is added to the README about interaction between job_interval_storage_stats and maintenance_window_start. - e.g.

With the default serverinfo config, if maintenance_window_start is enabled then the default update interval (3 hours) becomes once per 24 hour period. If you customize the interval while using maintenance_window_start the interval you choose must be <4 hours or storage updates may never run at all.

@kesselb kesselb marked this pull request as draft November 1, 2023 18:40
@kesselb
Copy link
Collaborator Author

kesselb commented Nov 1, 2023

Thank you, that's indeed a good point.

I guess the other jobs marked as time insensitive are indeed different. For some users, it might be important to have recent values for files and storage available via monitoring. For now, we should therefore keep the current approach.

@kesselb kesselb closed this Nov 1, 2023
@kesselb kesselb deleted the mark-background-job-as-time-insensitive branch November 1, 2023 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshtrichards joshtrichards joshtrichards approved these changes

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst

@JuliaKirschenheuter JuliaKirschenheuter Awaiting requested review from JuliaKirschenheuter

Assignees

@kesselb kesselb

Labels
3. to review enhancement
Projects
None yet
Milestone
Nextcloud 28
Development

Successfully merging this pull request may close these issues.
2 participants
@kesselb @joshtrichards